-
Notifications
You must be signed in to change notification settings - Fork 293
Implements transfer_locked_position instruction in ts-sdk #940
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
4664190 to
7af6810
Compare
| } | ||
| }); | ||
|
|
||
| const testTransferLockedPositionInstructions = async ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I intended to test following
- position owner opens new position
- lock it
- try to transfer locked position
- after transfer, verify if position is exsit, and locked.
7af6810 to
1ca7fcd
Compare
|
No lockPosition test code, so need to test it first. |
097394e to
757b2b3
Compare
|
|
||
| for (const poolName of poolTypes.keys()) { | ||
| for (const positionTypeName of positionTypes.keys()) { | ||
| const positionNameTE = `TE ${poolName} ${positionTypeName}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question) It seems if we open position with setupPosition(not supporting token extension), we couldn't lock position because it's token mint will be legacy SPL.
Are there any possible effects in real service? Like, if users open their position with open_position_instruction (not open_position_instruction_with_token_extension, they might not be able to lock position.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't there a setupTEPosition function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yap, I tested only positions made with setupTEPosition. But what I am wondering is that if we need to support lockPosition with position made with setupPosition. This case was failed. (So I didn't add this case.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recommend that. we should have a test case to verify that non-TE tokens cannot use transfer lock just to be complete.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added!
5377182 to
0e7cff2
Compare
wjthieme
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there! Couple of small comments left
| @@ -0,0 +1,154 @@ | |||
| import { describe, it, beforeAll } from "vitest"; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this from the other PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I include lockPosition test case in this PR. Because lockPosition and transferLockedPosition share same logics, but there is no lockPosition test code.
| tokenProgram: positionMint.programAddress, | ||
| }); | ||
|
|
||
| const lockConfigPda = await getProgramDerivedAddress({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wjthieme Could we use this with @orca-so/whirlpool-client? I think it is not exported. Or, I will move this to utils.ts when I fix lockPosition.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah if the function is not yet in @orca-so/whirlpool-client (or not exported) feel free to add/change it there!
60750f4 to
b3efc44
Compare
| for (const poolName of poolTypes.keys()) { | ||
| for (const positionTypeName of positionTypes.keys()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we try to use test.for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are several codes using this pattern, so how about fix them together?
|
|
||
| for (const poolName of poolTypes.keys()) { | ||
| for (const positionTypeName of positionTypes.keys()) { | ||
| const positionNameTE = `TE ${poolName} ${positionTypeName}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recommend that. we should have a test case to verify that non-TE tokens cannot use transfer lock just to be complete.
| ["one sided B", { tickLower: 1, tickUpper: 100 }], | ||
| ]); | ||
|
|
||
| describe("LockPosition instructions", () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest we add more test cases to cover all the parameter that this function takes in:
ex:
- receiver (self vs un-related wallet)
- authority (funder vs another wallet that owns the position vs another wallet that doesn't own the position)
- position mint types (TE vs non-TE) ✅
- position lock state (locked vs unlocked) ✅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make more tc about case 2, but could you describe more about case 1? is there receiver in lock-position?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry to clarify, this function takes in the receiver as where the locked position can be transferred to. Assuming walletA owns the locked position today, edge cases are:
- What if walletA sets walletA as receiver? (tries to send to self)
- walletA -> walletB (normal case)
afb9132 to
222c93e
Compare
222c93e to
c59ceda
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you apply the following two changes?
The PR has been open for a while, so I’d like to make only these changes and then merge it.
Export lockConfig.ts in ts-sdk/client/src/pda/index.ts and(PR1034 did it) usegetLockConfigAddressto derive lock config PDA.- Add an instruction to idempotently initialize the destinationTokenAccount (ATA).
Since @orca-so/whirlpools provides high-level functions, it will create required ATAs. However, transfer_locked_position instruction does not initialize the destination in the instruction itself, so I think it’s better to provide an ATA initialization instruction on the client side.
Regarding test cases, scenarios blocked at the contract level (e.g., non-TE positions cannot be locked or transferred) are already covered in the legacy-sdk integration tests.
(There’s no need to remove them.) It’s not mandatory for ts-sdk tests to verify that the contract rejects these cases.
This PR is implementing
transfer_locked_positioninstruction in ts-sdk.